feat(studio): flat inspector — Media group#2123
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Final current-head pass: 4b9c3cf. Reviewed the stack boundaries and current diff against the flat-inspector contracts; required checks have no failures/pending checks and no unresolved review threads remain. No new drift found; residual notes are non-blocking.
bb1be1f to
906d740
Compare
4b9c3cf to
523c461
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.
906d740 to
9c48c00
Compare
523c461 to
69c2dca
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 69c2dca46bc0499aa62a164b785b4a9d0285bdc2. The flat Media replacement changes established edit granularity: propertyPanelFlatMediaSection.tsx:204-223 relies on FlatSlider’s default step of 1, while the legacy media controls at propertyPanelMediaSection.tsx:260-287 use rate step 5 and media-start step 10. That is a silent interaction-contract regression in the layer that replaces the surface.
Pass the intended domain steps explicitly and pin them with parity tests.
Verdict: REQUEST CHANGES
Reasoning: The replacement Media controls silently change rate/start adjustment granularity from the legacy contract.
— Deepwork
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head . The previously reported Media step-contract regression remains unchanged.\n\nStrength: The Media layer keeps and writes routed through the same style persistence boundary ().\n\n**[blocker] The new Position select cannot represent authored CSS positions.** passes arbitrary into but offers only nine presets. The primitive renders no fallback option for the current value, so common valid values such as , , or display as . The user cannot distinguish custom persisted positioning from the default and can overwrite it unknowingly. Preserve unknown current values, matching legacy , and pin a custom-position regression.\n\nVerdict: REQUEST CHANGES\nReasoning: This layer introduces a concrete consumer where the flat select loses valid authored state, in addition to the still-unfixed media granularity regression.\n\n— Deepwork
Formatting was corrupted during submission; replacing with corrected review.
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head 69c2dca46bc0499aa62a164b785b4a9d0285bdc2. The previously reported Media step-contract regression remains unchanged.
Strength: Media routes object-fit and object-position through the same style persistence boundary (propertyPanelFlatMediaSection.tsx:251-276).
[blocker] The Position select cannot represent authored CSS positions. propertyPanelFlatMediaSection.tsx:260-275 passes arbitrary objectPosition into FlatSelectRow but offers nine presets. Values such as 25% 70%, 10px 2em, or right 12px bottom 4px display as center. The user can unknowingly overwrite valid persisted positioning. Preserve unknown values, matching legacy SelectField, and pin a custom-position regression.
Verdict: REQUEST CHANGES
Reasoning: This layer adds a concrete consumer where the flat select loses valid authored state, in addition to the still-unfixed media granularity regression.
— Deepwork
9c48c00 to
7a3c7ad
Compare
69c2dca to
1eb9ab3
Compare
|
Fixed in PR #2416, commit Same root cause as the Style/Blend finding on #2121: New regression test on the primitive: |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at 1eb9ab356f16e20bd289ed086a27d1a4d77b448e.
The incremental patch is byte-equivalent to the previously reviewed head (patch-id b2d0be44…). propertyPanelFlatMediaSection.tsx:260-275 still feeds arbitrary authored object-position values into a fixed preset select, so values such as 25% 70% display as a different preset and can be overwritten. Media Rate and Media start also still omit the legacy step behavior at :195-223, and the copy-feedback timeout remains untracked across unmount.
Verdict: REQUEST CHANGES
Reasoning: This force-push is a rebase only; the media value-parity and lifecycle blockers are unchanged.
— Deepwork
|
Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack ( This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite ( Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack-scoped re-review at 1eb9ab3.
The incremental layer is unchanged, and its prior requested changes are now resolved in the accepted mandatory tip fix, #2416 at 832d5be. I verified the cumulative tip closes the flat-inspector identity, value-parity, lifecycle, persistence, accessibility, and constrained-layout findings with focused regression coverage. This approval assumes the Graphite stack lands bottom-up through #2416; this layer should not be landed while dropping the approved tip fix.
Verdict: APPROVE
Reasoning: The accepted tip-fix strategy closes the blocker inherited or introduced at this layer, and the verified cumulative stack is coherent through #2416.
— Deepwork
## What First PR in a 6-PR stack migrating Studio's right-panel property inspector from an always-expanded stacked-sections layout to a "flat" one-open-at-a-time accordion. This PR lays the foundation: the `STUDIO_FLAT_INSPECTOR_ENABLED` feature flag, the accordion primitives (`FlatRow`, `FlatSegmentedRow`, `FlatGroup`, `PinnedZoneDivider`), the flat identity header/footer, and the first migrated group — Text. Stack: #2120 (this) → #2121 (Style) → #2122 (Layout+Motion) → #2123 (Media) → #2124 (Grade) → #2125 (Pinning + multi-field Text). ## Why The legacy inspector renders every applicable section expanded at once, which gets unwieldy as an element accumulates properties across style/layout/motion/media/grade. The flat redesign shows one section at a time (plus pinned sections), matching a design handoff mock. ## How - `FlatGroup` owns the one-open accordion state (`openGroupId`/`onToggleOpen`) and pin affordance (`onTogglePin`), styled per the design mock. - `FlatTextSection` is the first migrated group and the reference implementation every later group's task followed for the `isOpen`/`onToggleOpen`/`onTogglePin`/`summary` wiring pattern. - Includes a same-PR bugfix (found via live browser testing, not caught by any automated test): the Text `FlatGroup` was rendering unconditionally regardless of element type (empty for non-text elements), and the multi-field fallback doubled the "Text" heading. Fixed by gating on `isTextEditableSelection` and adding a `hideOwnHeading` prop to the legacy `TextSection` fallback. - Entirely gated behind `STUDIO_FLAT_INSPECTOR_ENABLED` (default off) — the legacy panel is untouched and remains the default for all users. ## Test plan - Every primitive and the Text group have dedicated Vitest suites using real DOM events (click/pointerdown) with exact assertions, not shallow snapshots. - Manually verified in Studio via live browser testing against the design mock (this is what caught the bugfix above). - Full monorepo test suite green; `oxlint`/`oxfmt` clean; this repo's `fallow` complexity/duplication gate passes. - [x] Unit tests added/updated - [x] Manual testing performed - [ ] Documentation updated (not applicable — internal Studio UI behind an off-by-default flag)
1eb9ab3 to
5a3abde
Compare
7a3c7ad to
4b59283
Compare
The base branch was changed.

What
Fourth PR in a 6-PR stack for the flat inspector redesign (see #2120 for the foundation and full stack list). Adds the
FlatToggleprimitive and the flat Media group (source/copy row, cutout sub-block, volume/rate/media-start sliders, loop/muted/has-audio-track toggles, fit/position rows), wired into the accordion.Stack: #2120 (Foundation+Text) → #2121 (Style) → #2122 (Layout+Motion) → #2123 (this) → #2124 (Grade) → #2125 (Pinning + multi-field Text).
Why
Media covers every video/audio/image-specific control (playback, cutout, fit) that doesn't belong in the more general Style/Layout groups.
How
FlatToggleis a new shared primitive (used here for loop/muted/has-audio-track, and reused by the Grade group in the next PR).FlatMediaSectioncomposes six rows/sub-blocks across playback and framing controls.STUDIO_FLAT_INSPECTOR_ENABLED(default off).Test plan
onSetHtmlAttributecalls), which required an explicit fix-round during task review to reach full coverage.oxlint/oxfmt/fallowclean.